Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Require package names to be non-empty #2293

Merged
merged 7 commits into from
Aug 14, 2023
Merged

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Aug 11, 2023

Description

Problem*

Resolves #2287

Summary*

This PR requires that CrateNames must have a non-empty string to avoid the issue described in #2287

We also weren't validating these on package creation, so I've added a custom value parser to clap in order to error on invalid user input as soon as possible so we don't get nargo new half-creating the new package.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

BEGIN_COMMIT_OVERRIDE
fix: Require package names to be non-empty (#2293)
feat: Perform input validation on user's package names (#2293)
END_COMMIT_OVERRIDE

@TomAFrench TomAFrench requested a review from phated August 11, 2023 18:25
@TomAFrench TomAFrench changed the title fix: reject invalid CrateNames in clap fix: Require package names to be non-empty Aug 11, 2023
Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be implemented in the FromStr trait on CrateName

Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things, but looking good

crates/nargo_cli/src/cli/init_cmd.rs Outdated Show resolved Hide resolved
crates/nargo_cli/src/cli/new_cmd.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/graph/mod.rs Show resolved Hide resolved
crates/noirc_frontend/src/graph/mod.rs Outdated Show resolved Hide resolved
@phated
Copy link
Contributor

phated commented Aug 11, 2023

Should we add some failing integration tests for bad package names in Nargo.toml?

@TomAFrench
Copy link
Member Author

Should we add some failing integration tests for bad package names in Nargo.toml?

Done

@TomAFrench TomAFrench requested a review from phated August 14, 2023 08:47
@TomAFrench TomAFrench added this pull request to the merge queue Aug 14, 2023
Merged via the queue into master with commit 87174ac Aug 14, 2023
15 checks passed
@TomAFrench TomAFrench deleted the package-name-validation branch August 14, 2023 16:08
TomAFrench added a commit that referenced this pull request Aug 14, 2023
* master:
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
TomAFrench added a commit that referenced this pull request Aug 15, 2023
* master:
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  feat: Include struct names in ABIs (#2266)
  feat(nargo): Add `--exact` flag to `nargo test` (#2272)
TomAFrench added a commit that referenced this pull request Aug 15, 2023
* master: (23 commits)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  feat: Include struct names in ABIs (#2266)
  feat(nargo): Add `--exact` flag to `nargo test` (#2272)
  fix: Fix assignment when both `mut` and `&mut` are used (#2264)
  feat: Add `assert_constant` (#2242)
  feat(nargo): Add support for contracts in `nargo check` (#2267)
  chore(ci): Name wasm job more clearly (#2269)
  chore(ci): Create cache key with consideration to target (#2273)
  chore(ci): Run publish workflow against PRs (#2268)
  ...
TomAFrench added a commit that referenced this pull request Aug 15, 2023
* master: (26 commits)
  chore(noir): Release 0.10.0 (#2039)
  fix(lsp): Ensure lsp does not crawl past the root specified (#2322)
  fix: Prevent panic when passing relative paths to `--program-dir` (#2324)
  fix: Overflowing assignment will result in an error (#2321)
  chore: clippy fixes (#2320)
  chore: Parameterize the build mode for noir-wasm (#2317)
  chore: Make `wasm` tests pull from `result` directory (#2319)
  chore: Fix typo (#2315)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CrateName does not reject empty strings
2 participants